Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't self-test ECJPAKE ALT implementations against known entropy #4007

Conversation

stevew817
Copy link
Contributor

Description

These implementations don't necessarily consume entropy the same way the mbed TLS internal software implementation does, and the 'reference handshake' test vectors can thus not be applied to an ALT implementation.

Status

READY

Requires Backporting

NO

Migrations

NO

@gabor-mezei-arm gabor-mezei-arm added Community needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jan 12, 2021
@gabor-mezei-arm gabor-mezei-arm added the component-crypto Crypto primitives and low-level interfaces label Jan 12, 2021
@gilles-peskine-arm gilles-peskine-arm removed their request for review January 18, 2021 13:09
@chris-jones-arm chris-jones-arm self-requested a review January 18, 2021 17:29
Copy link
Contributor

@chris-jones-arm chris-jones-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, I just need some clarification on a few points that I may have misunderstood.

library/ecjpake.c Show resolved Hide resolved
library/ecjpake.c Show resolved Hide resolved
Copy link
Contributor

@chris-jones-arm chris-jones-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I still think this change is worth backporting to 2.16 however to keep the code identical in both branches and keep it maintainable. There should be no need to backport to 2.7 as it is entering end of life and this change is non-critical.

@chris-jones-arm chris-jones-arm added needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 19, 2021
@@ -1059,6 +1063,7 @@ int mbedtls_ecjpake_self_test( int verbose )
if( verbose != 0 )
mbedtls_printf( "passed\n" );

#if !defined(MBEDTLS_ECJPAKE_ALT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining why those tests are not relevant in the case of an alternative implementation of ECJPAKE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -820,6 +820,8 @@ static const unsigned char ecjpake_test_password[] = {
0x65, 0x73, 0x74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: the commit message is too long and not rendered properly by GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

These implementations don't necessarily consume entropy the same way the
mbed TLS internal software implementation does, and the 'reference
handshake' test vectors can thus not be applied to an ALT implementation.

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the very good and clear additional comment.

Copy link
Contributor

@chris-jones-arm chris-jones-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just needs a backport as previously mentioned and then this is good to merge.

@chris-jones-arm chris-jones-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 22, 2021
@stevew817
Copy link
Contributor Author

@chris-jones-arm backport available in #4057.

@chris-jones-arm chris-jones-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Jan 25, 2021
@yanesca yanesca merged commit b034683 into Mbed-TLS:development Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants